Add support for configuring domains starting with a number via environment variables (by using _ prefix)#3414
Add support for configuring domains starting with a number via environment variables (by using _ prefix)#3414ppenguin wants to merge 2 commits intobunkerity:devfrom
_ prefix)#3414Conversation
To support domains starting with a number configured through env vars, we need to prefix them with _ and allow such variables to be handled for config generation. fix env var strip
✅ Actions performedReview triggered.
|
src/commonConfiguration variable normalization for numeric-prefixed domain names
User-visible behaviour change: Users can now configure numeric-prefixed domains via environment variables by prefixing the variable name with underscore (e.g., Configuration/schema changes: No schema changes; this is a transparent input normalisation feature within the existing Configurator class. Documentation/tests: No documentation or tests were updated in this commit. Security impact: None identified. The normalisation is a purely mechanical string transformation applied early in processing, before validation. WalkthroughThe change implements underscore-prefixed environment variable stripping in Configurator to enable configuration of domain names starting with numerals. Variables with leading underscores are normalised before downstream processing, with collision detection logging warnings when multiple keys map to the same stripped key. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Out-of-scope changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common/gen/Configurator.py`:
- Around line 114-127: The collision warning in Configurator (where
self.__variables is processed and assigned to stripped) can report misleading
names because it only shows the post-strip key; update the logic to track the
original source key for each entry (e.g., store new_key -> (orig_key, value)
while building stripped) so that when you detect a collision you can call
self.__logger.warning with both the previously stored orig_key and the current
k, and then store/overwrite the value with the current one before finally
collapsing the mapping back to new_key -> value for self.__variables; adjust the
warning text accordingly to show both original keys and that the last value is
kept.
- Around line 118-127: The collision handling in the loop over self.__variables
uses "last wins" which is iteration-order dependent; change it to a
deterministic rule that always prefers the explicit non-underscore key over the
underscore-prefixed form: when computing new_key = k.removeprefix("_") and
encountering new_key in stripped, check whether the existing entry in stripped
came from a non-prefixed original (i.e., the existing source key did not start
with "_") and if so keep the existing value and do not overwrite; otherwise
(existing was from a prefixed key and current k is non-prefixed) overwrite;
update the warning via self.__logger.warning accordingly and ensure the logic
references __variables, new_key, stripped, and removeprefix so behavior is
deterministic regardless of iteration order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3cadd0b-0947-4bb4-8ea8-3a25f14f6ba1
📒 Files selected for processing (1)
src/common/gen/Configurator.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Follow BunkerWeb's Python standards and security posture:
- Use snake_case for functions and variables, PascalCase for classes, and provide concise, accurate docstrings for public classes, functions, and methods.
- Respect Black formatting with a 160-character line limit and the existing pre-commit conventions. Do not insist on adding type annotations to previously untyped code, but accept them when added consistently.
- Catch specific exceptions; never use bare
except:. CatchingExceptionis acceptable only at explicit process boundaries (for example scheduler loops, outer job runners, worker entrypoints, or graceful-shutdown boundaries) when the code logs enough context and either re-raises, returns an error status, or terminates safely.- Never use
os.system,subprocess.*(..., shell=True),eval, orexec. Pass subprocess arguments as a list and prefer explicit binary paths for privileged operations.- Do not use unsafe deserialisers (
pickle,marshal,shelve,jsonpickle,dill) for untrusted data. Useyaml.safe_load()rather than unsafe YAML loading.- Open files with an explicit encoding (normally
utf-8) and usewithstatements for files, sockets, database sessions, and temporary resources.- Use
secretsfor token generation andhmac.compare_digestfor token, HMAC, or signature comparisons.- For HTTP clients (
requests,httpx): always set an explicit timeout, validate destination URLs to block RFC1918/loopback/link-local ranges (SSRF), disable automatic redirects to internal hosts, and be careful with proxy settings.- For filesystem operations: resolve paths with
Path.resolve()and verify they remain under the intended base directory before reading or writing (path traversal).- Use
defusedxmlrather than stdlib XML parsers for untrusted XML.- For SQLAlchemy, use bound parameters and safe query construction. Never call
text()with f-strings or.format(), and flag.execute()calls built...
Files:
src/common/gen/Configurator.py
src/common/gen/**/*.py
⚙️ CodeRabbit configuration file
src/common/gen/**/*.py:src/common/gen/drives settings validation and config generation:
- Preserve determinism: the same validated settings should render the same output.
- Do not bypass the configurator or validation layer when introducing new settings.
- Any template change that depends on a new setting should come with the corresponding schema or plugin metadata update.
- Be careful with escaping and quoting so user-supplied settings cannot break out of generated directives or file formats.
Files:
src/common/gen/Configurator.py
🔇 Additional comments (1)
src/common/gen/Configurator.py (1)
52-52: LGTM on excluded-prefixes update.Removing
_from__excluded_prefixesallows underscore-prefixed variables through for subsequent stripping, and addingNOMAD_reduces log noise in Nomad deployments. Both changes align with the PR objectives.
| # Allow a leading underscore prefix on env var names so that domain names | ||
| # starting with a digit can be configured (bash forbids var names starting | ||
| # with a digit, e.g. 1nteresting.io). Users write _1nteresting.io_USE_... | ||
| # and we strip the single leading underscore here before any other processing. | ||
| stripped: Dict[str, str] = {} | ||
| for k, v in self.__variables.items(): | ||
| new_key = k.removeprefix("_") | ||
| if not new_key: | ||
| # Skip bare "_" or similar empty-after-strip keys | ||
| continue | ||
| if new_key in stripped: | ||
| self.__logger.warning(f"Variable collision after stripping leading underscore: both {k!r} and {new_key!r} exist, keeping last value") | ||
| stripped[new_key] = v | ||
| self.__variables = stripped |
There was a problem hiding this comment.
Collision warning message is misleading when the non-prefixed key is processed second.
When iteration order means _FOO is processed before FOO, the stripped key FOO is already in stripped. The current log reads:
"both 'FOO' and 'FOO' exist, keeping last value"
This doesn't identify the original underscore-prefixed key that caused the collision. Consider tracking which original key produced each entry in stripped so the warning can report both original names accurately.
🔧 Suggested fix
- stripped: Dict[str, str] = {}
+ stripped: Dict[str, str] = {}
+ stripped_origins: Dict[str, str] = {} # maps new_key -> original key
for k, v in self.__variables.items():
new_key = k.removeprefix("_")
if not new_key:
# Skip bare "_" or similar empty-after-strip keys
continue
if new_key in stripped:
- self.__logger.warning(f"Variable collision after stripping leading underscore: both {k!r} and {new_key!r} exist, keeping last value")
+ self.__logger.warning(
+ f"Variable collision after stripping leading underscore: "
+ f"{stripped_origins[new_key]!r} and {k!r} both map to {new_key!r}, keeping value from {k!r}"
+ )
stripped[new_key] = v
+ stripped_origins[new_key] = k
self.__variables = stripped🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/gen/Configurator.py` around lines 114 - 127, The collision warning
in Configurator (where self.__variables is processed and assigned to stripped)
can report misleading names because it only shows the post-strip key; update the
logic to track the original source key for each entry (e.g., store new_key ->
(orig_key, value) while building stripped) so that when you detect a collision
you can call self.__logger.warning with both the previously stored orig_key and
the current k, and then store/overwrite the value with the current one before
finally collapsing the mapping back to new_key -> value for self.__variables;
adjust the warning text accordingly to show both original keys and that the last
value is kept.
| stripped: Dict[str, str] = {} | ||
| for k, v in self.__variables.items(): | ||
| new_key = k.removeprefix("_") | ||
| if not new_key: | ||
| # Skip bare "_" or similar empty-after-strip keys | ||
| continue | ||
| if new_key in stripped: | ||
| self.__logger.warning(f"Variable collision after stripping leading underscore: both {k!r} and {new_key!r} exist, keeping last value") | ||
| stripped[new_key] = v | ||
| self.__variables = stripped |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
"Last wins" collision resolution is iteration-order dependent.
As per coding guidelines for src/common/gen/, determinism should be preserved—the same input should produce the same output. When both FOO and _FOO exist, the surviving value depends on dict iteration order, which may vary if variables originate from environment (historically unordered on older Pythons or external orchestrators) rather than file parsing.
Consider a deterministic tie-breaker, such as always preferring the explicit (non-underscore) key over the underscore-prefixed form.
♻️ Possible deterministic approach
for k, v in self.__variables.items():
new_key = k.removeprefix("_")
if not new_key:
continue
- if new_key in stripped:
- self.__logger.warning(f"Variable collision after stripping leading underscore: both {k!r} and {new_key!r} exist, keeping last value")
- stripped[new_key] = v
+ if new_key in stripped:
+ # Prefer explicit (non-prefixed) key; only overwrite if current key is non-prefixed
+ if k == new_key:
+ self.__logger.warning(
+ f"Variable collision: {new_key!r} overrides previously stripped key"
+ )
+ stripped[new_key] = v
+ else:
+ self.__logger.warning(
+ f"Variable collision: keeping explicit {new_key!r}, ignoring {k!r}"
+ )
+ else:
+ stripped[new_key] = v📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stripped: Dict[str, str] = {} | |
| for k, v in self.__variables.items(): | |
| new_key = k.removeprefix("_") | |
| if not new_key: | |
| # Skip bare "_" or similar empty-after-strip keys | |
| continue | |
| if new_key in stripped: | |
| self.__logger.warning(f"Variable collision after stripping leading underscore: both {k!r} and {new_key!r} exist, keeping last value") | |
| stripped[new_key] = v | |
| self.__variables = stripped | |
| stripped: Dict[str, str] = {} | |
| for k, v in self.__variables.items(): | |
| new_key = k.removeprefix("_") | |
| if not new_key: | |
| # Skip bare "_" or similar empty-after-strip keys | |
| continue | |
| if new_key in stripped: | |
| # Prefer explicit (non-prefixed) key; only overwrite if current key is non-prefixed | |
| if k == new_key: | |
| self.__logger.warning( | |
| f"Variable collision: {new_key!r} overrides previously stripped key" | |
| ) | |
| stripped[new_key] = v | |
| else: | |
| self.__logger.warning( | |
| f"Variable collision: keeping explicit {new_key!r}, ignoring {k!r}" | |
| ) | |
| else: | |
| stripped[new_key] = v | |
| self.__variables = stripped |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/gen/Configurator.py` around lines 118 - 127, The collision
handling in the loop over self.__variables uses "last wins" which is
iteration-order dependent; change it to a deterministic rule that always prefers
the explicit non-underscore key over the underscore-prefixed form: when
computing new_key = k.removeprefix("_") and encountering new_key in stripped,
check whether the existing entry in stripped came from a non-prefixed original
(i.e., the existing source key did not start with "_") and if so keep the
existing value and do not overwrite; otherwise (existing was from a prefixed key
and current k is non-prefixed) overwrite; update the warning via
self.__logger.warning accordingly and ensure the logic references __variables,
new_key, stripped, and removeprefix so behavior is deterministic regardless of
iteration order.
3 participants
Closes #1857
Allows prefixing environment variables with
_with the main purpose to allow (v)host/domain names starting with a number (e.g.1mthe1.org->_1mthe1.org_USE_LETS_ENCRYPT_WILDCARD="yes").The
_prefix has been removed from the exclude prefixes list, but the_prefix is stripped from the variable name(s) as early as possible, so all further logic handles the server names etc. unchanged, i.e. as1mthe1.orgin this example.Also adds
NOMAD_to the exluded prefixes list to make startup in nomad deployments less noisy.